TRUNK-6469: Add support for automatic initialization of Envers audit tables (#5468) (#5831) by wikumChamith · Pull Request #5834 · openmrs/openmrs-core

wikumChamith · 2026-02-23T07:34:04Z

Description of what I changed

Cherry picked from: b276e07

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

omeriinnocent

The implementation of automatic Envers audit table initialization is solid:
Strengths:
- Well-structured code with proper separation of concerns in EnversAuditTableInitializer
- Comprehensive test coverage (3 main scenarios: enable/disable/custom suffix)
- Follows OpenMRS standards (license headers, naming, logging with SLF4J)
- Good error handling with APIException wrapping
- Java 8 compatible (DatabaseUtil.java fix is good)
Minor Recommendations:
-In HibernateSessionFactoryBean.generateEnversAuditTables(), consider adding a null check for hibernateProperties before passing to EnversAuditTableInitializer.initialize() for defensive programming.
-Add javadoc to the test helper method buildSessionFactoryWithEnvers() for clarity.

ibacher · 2026-02-24T16:31:41Z

@omeriinnocent Please don't add AI-generated review comments unless they add some kind of value. While it's fine to use AI to help you identify issues that you might want to raise with a PR author in a review, just copying and pasting the AI output is unhelpful.

ibacher · 2026-02-24T16:32:39Z

api/src/main/java/org/openmrs/api/db/hibernate/HibernateSessionFactoryBean.java

 import org.hibernate.integrator.spi.Integrator;
+import org.hibernate.service.ServiceRegistry;
 import org.hibernate.service.spi.SessionFactoryServiceRegistry;
+import org.jspecify.annotations.NonNull;


Prefer @Nonnull or at least the Spring version.

This is the one used by the parent class: spring-projects/spring-framework@2691489/spring-orm/src/main/java/org/springframework/orm/jpa/hibernate/LocalSessionFactoryBean.java#L39

ibacher · 2026-02-24T16:33:06Z

api/src/main/java/org/openmrs/api/db/hibernate/HibernateSessionFactoryBean.java

 	 */
 	@Override
-	public void setMappingResources(String... mappingResources) {
+	public void setMappingResources(String @NonNull ... mappingResources) {


Suggested change

public void setMappingResources(String @NonNull ... mappingResources) {

public void setMappingResources(@Nonnull String ... mappingResources) {

@ibacher Since this method uses varargs, we need to annotate it as String @NonNull ..., which means the array itself cannot be null.

If we annotate it as @NonNull String ..., that means each String element cannot be null.

When I used @NonNull String ..., I got the warning:
Not annotated parameter overrides @NonNullApi parameter.

That’s why I annotated it as String @NonNull ... instead.

wikumChamith · 2026-03-04T05:57:49Z

@ibacher, @dkayiwa is this ready to get merged :) ?

…tables (openmrs#5468) (openmrs#5831)

sonarqubecloud · 2026-03-12T05:28:06Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Binayak490-cyber · 2026-03-22T13:22:45Z

api/src/main/java/org/openmrs/ConceptReferenceRange.java

 	private Integer conceptReferenceRangeId;

-	@Column(name = "criteria", length = 65535)
+	@Column(name = "criteria", length = 255)


@wikumChamith, as I was also working on the same issue around and they are #5933 and #5952 and was finding this PR only where the backfill changeset only makes sense if the audit tables already exist. So, I want to point out some points that the criteria column was silently reduced from 65535 to 255 with no Liquibase migration — any existing values longer than 255 chars would be truncated on update. Was this intentional? A migration changeset is needed here.

Binayak490-cyber · 2026-03-22T13:23:56Z

api/src/main/java/org/openmrs/util/EnversAuditTableInitializer.java

+
+				String lowerTableName = tableName.toLowerCase();
+
+				if (lowerTableName.contains("revision") || lowerTableName.equals("revinfo")) {


@wikumChamith, Any table whose name happens to contain the word "revision" would be incorrectly pulled into the audit schema migration. This is too broad — should match only the exact Envers-generated revision table name based on the configured prefix/suffix.

Binayak490-cyber · 2026-03-22T13:26:00Z

api/src/main/java/org/openmrs/util/EnversAuditTableInitializer.java

+					hasErrors.set(true);
+					log.warn("Schema migration encountered an issue: {}", throwable.getMessage());


Errors during audit table creation are only logged as warnings and execution continues. If a critical table fails to create, the system starts up silently broken — this should either throw or at minimum log at ERROR level so it doesn't go unnoticed in production.

Binayak490-cyber · 2026-03-22T13:28:31Z

api/src/main/java/org/openmrs/util/EnversAuditTableInitializer.java

+		String auditTablePrefix = hibernateProperties.getProperty("org.hibernate.envers.audit_table_prefix", "");
+		String auditTableSuffix = hibernateProperties.getProperty("org.hibernate.envers.audit_table_suffix", "_audit");
+
+		Map<String, Object> settings = new HashMap<>((Map) hibernateProperties);


This is an unchecked raw type cast that will generate a compiler warning and could throw a ClassCastException at runtime if any property value is not a String. Should use explicit population instead.

Binayak490-cyber · 2026-03-22T13:29:06Z

api/src/test/java/org/openmrs/util/databasechange/EnversAuditTableInitializerDatabaseIT.java

+
+	@BeforeEach
+	void beforeEach() throws Exception {
+		this.dropAllDatabaseObjects();


Calling dropAllDatabaseObjects() in @beforeeach means if one test fails mid-run, the DB is left in a broken state for all subsequent tests. A more targeted teardown or transaction rollback would be safer.

Binayak490-cyber · 2026-03-22T13:29:41Z

api/src/test/java/org/openmrs/util/databasechange/EnversAuditTableInitializerDatabaseIT.java

+
+	@BeforeEach
+	void beforeEach() throws Exception {
+		this.dropAllDatabaseObjects();


Calling dropAllDatabaseObjects() in @beforeeach means if one test fails mid-run, the DB is left in a broken state for all subsequent tests. A more targeted teardown or transaction rollback would be safer.

Binayak490-cyber · 2026-03-22T13:29:51Z

api/src/main/java/org/openmrs/api/db/hibernate/HibernateSessionFactoryBean.java

 	public void integrate(Metadata metadata, SessionFactoryImplementor sessionFactory,
 			SessionFactoryServiceRegistry serviceRegistry) {
 		this.metadata = metadata;
+		generateEnversAuditTables(metadata, serviceRegistry);


Throwing APIException from inside integrate() will crash the entire SessionFactory initialization with a generic error — giving no clear signal that Envers is the cause. Should log the failure clearly and consider making it non-fatal so startup can proceed.

wikumChamith requested review from dkayiwa and ibacher February 23, 2026 09:13

omeriinnocent reviewed Feb 24, 2026

View reviewed changes

ibacher reviewed Feb 24, 2026

View reviewed changes

wikumChamith force-pushed the 2.8.x-envers branch 2 times, most recently from 5b232de to 6effc2a Compare March 2, 2026 06:44

wikumChamith requested a review from ibacher March 11, 2026 12:32

TRUNK-6469: Add support for automatic initialization of Envers audit …

c08801a

…tables (openmrs#5468) (openmrs#5831)

wikumChamith force-pushed the 2.8.x-envers branch from 415fdff to c08801a Compare March 12, 2026 05:26

Binayak490-cyber reviewed Mar 22, 2026

View reviewed changes

	public void setMappingResources(String @NonNull ... mappingResources) {
	public void setMappingResources(@Nonnull String ... mappingResources) {


		String lowerTableName = tableName.toLowerCase();

		if (lowerTableName.contains("revision") \|\| lowerTableName.equals("revinfo")) {

		hasErrors.set(true);
		log.warn("Schema migration encountered an issue: {}", throwable.getMessage());

Conversation

wikumChamith commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

Uh oh!

omeriinnocent left a comment

Choose a reason for hiding this comment

Uh oh!

ibacher commented Feb 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wikumChamith Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wikumChamith commented Mar 4, 2026

Uh oh!

sonarqubecloud bot commented Mar 12, 2026

Quality Gate passed

Uh oh!

Binayak490-cyber Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wikumChamith commented Feb 23, 2026 •

edited

Loading

wikumChamith Feb 25, 2026 •

edited

Loading

Binayak490-cyber Mar 22, 2026 •

edited

Loading